Refactor DefaultFieldCustomizer and TableViewRowRenderer to improve component configuration#52
Conversation
…omponent class handling and defaults
There was a problem hiding this comment.
Pull request overview
This pull request refactors component configuration logic in the ZK viewers layer to improve consistency in how UI components are resolved from field specifications. The main changes consolidate component resolution logic by ensuring that both componentClass (Java Class) and component (String alias) fields are properly synchronized using the ComponentAliasIndex.
Changes:
- Refactored
DefaultFieldCustomizer.customize()to explicitly set bothcomponentClassandcomponentwhen defaulting to Label, maintaining bidirectional consistency between these fields - Refactored
TableViewRowRenderer.createFieldComponent()to handle component alias resolution more explicitly, checking bothcomponentClassandcomponentwith proper null handling - Removed
finalmodifiers fromEntityPickerBox.setEntityClass()methods to allow subclass customization
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| DefaultFieldCustomizer.java | Refactored field customization logic to ensure both componentClass and component fields are set consistently, adding explicit default to Label.class/"label" when both are null |
| TableViewRowRenderer.java | Improved component creation logic with clearer separation of componentClass and component alias resolution, with proper null handling |
| EntityPickerBox.java | Removed final modifiers from setEntityClass methods to allow method overriding in subclasses |
| if (field.getComponentClass() == null && field.getComponent() == null) { | ||
| field.setComponentClass(Label.class); | ||
| field.setComponent("label"); | ||
| } |
There was a problem hiding this comment.
The new behavior at lines 128-131 sets field.setComponent("label") in addition to setting field.setComponentClass(Label.class). This represents a change in behavior - previously, only componentClass was set to Label.class as a default.
While this change appears correct and maintains consistency between componentClass and component fields (as verified by test testDateboxLocalDate at line 111), there's no explicit test coverage for this specific code path. Consider adding a test that verifies when both componentClass and component are null for a non-form, non-boolean field, both get set to Label.class and "label" respectively.
| if (field.getComponent() != null && !field.getComponent().isBlank() && field.getComponentClass() == null) { | ||
| field.setComponentClass(ComponentAliasIndex.getInstance().get(field.getComponent())); | ||
| } | ||
|
|
There was a problem hiding this comment.
Redundant component alias resolution: The ComponentAliasIndex lookup at line 121 is redundant with an earlier lookup performed outside this conditional block (at line 103-105, before the field.isVisible() check).
If field.getComponent() was set and the alias exists, componentClass would already be set at line 104, preventing entry into this block. If the alias lookup at line 104 returned null (unrecognized alias), repeating the same lookup here at line 121 will also return null.
This redundancy doesn't cause incorrect behavior but adds unnecessary processing. Consider either: (1) removing lines 103-105 and keeping only this more thorough check that uses isBlank(), or (2) removing lines 120-122 since the alias resolution already happened earlier.
| if (field.getComponent() != null && !field.getComponent().isBlank() && field.getComponentClass() == null) { | |
| field.setComponentClass(ComponentAliasIndex.getInstance().get(field.getComponent())); | |
| } |
No description provided.